[ui] Graph Editor Update: Quick Node Coloring with the Color Selector Tool #2604
Conversation
The color selector in the Graph Editor provides a quick way to color nodes with predefined palette of colors. Added Command to allow the Coloring to be undone and redone using QUndoStack
yann-lty
left a comment
yann-lty
left a comment
There was a problem hiding this comment.
Implementation-wise: this should rely on the existing SetAttributeCommand.
UX-wise: I would suggest re-working the color palette for better contrast with the font color.
meshroom/ui/graph.py
Outdated
| self.selectNodes(self._graph.dfsOnDiscover(startNodes=[node], reverse=True, dependenciesOnly=True)[0]) | ||
|
|
||
| @Slot(str) | ||
| def updateNodeColor(self, color): |
There was a problem hiding this comment.
As this method is working on the node selection (which makes sense), I would recommend:
| def updateNodeColor(self, color): | |
| def setSelectedNodesColor(self, color: str): |
Current meshroom nodes have the FG text in gray and the background color needs to constrast well else the text becomes unreadable
yann-lty
left a comment
yann-lty
left a comment
There was a problem hiding this comment.
Minor fixes left, but LGTM after that 👍.
meshroom/core/node.py
Outdated
| return self.internalAttribute("color").value.strip() | ||
| return "" | ||
|
|
||
| def setColor(self, color: str): |
There was a problem hiding this comment.
With regards to what this PR is addressing, adding the setter for this property is not needed.
meshroom/core/node.py
Outdated
| internalAttributesChanged = Signal() | ||
| label = Property(str, getLabel, notify=internalAttributesChanged) | ||
| color = Property(str, getColor, notify=internalAttributesChanged) | ||
| color = Property(str, getColor, setColor, notify=internalAttributesChanged) |
There was a problem hiding this comment.
(same as above) Adding the setter is not needed in this scope.
There was a problem hiding this comment.
This has been removed.
meshroom/ui/graph.py
Outdated
| color (str): Hex code of the color to be set on the nodes. | ||
| """ | ||
| # Update the color attribute of the nodes which are currently selected | ||
| with self.groupedGraphModification("Update Color for Select Nodes"): |
There was a problem hiding this comment.
For the command name that appears in the undo stack:
| with self.groupedGraphModification("Update Color for Select Nodes"): | |
| with self.groupedGraphModification("Set Nodes Color"): |
I think we could go with removing the notion of "selected" in those, as selection operations are not undoable - therefore, undoing "Set Selected Nodes Color" is a bit ambiguous if the selection has changed in between.
What do you think?
There was a problem hiding this comment.
Yes, makes sense, I probably did not notice this while working on the previous comments. Thanks for the suggestion.
Coloring the nodes now uses setAttribute command instead of relying on refrences to Nodes within the graph.
waaake
force-pushed
the
dev/NodeColoring
branch
from
2d56c78 to
233bbb7
Compare
yann-lty
left a comment
yann-lty
left a comment
There was a problem hiding this comment.
The "C" shortcut is currently breaking the Copy action (Ctrl+C).
| Keys.onPressed: function(event) { | ||
| if (event.key === Qt.Key_F) { | ||
| fit() | ||
| } else if (event.key === Qt.Key_C) { |
There was a problem hiding this comment.
This takes precedence over the copy shortcut (Ctrl+C).
We need better handling of keyboard shortcuts, but in the meantime, this should either test if no modifier are active, or be moved at after the copy action handling.
There was a problem hiding this comment.
Yeah, it's my bad, I did not notice something as simple as this. Have updated the code to consider both cases.
fabiencastan
added
the
feature
Description
Added Color Selector in the Graph Editor
Features list
Implementation remarks